Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add $INCLUDE parsing and RFC 2308 $TTL compliance #2

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add $INCLUDE parsing and RFC 2308 $TTL compliance #2

wants to merge 11 commits into from

Conversation

xdrr
Copy link

@xdrr xdrr commented Oct 20, 2014

This pull request contains both the $INCLUDE parsing functionality and RFC 2308 $TTL complaince changes into the dns-zoneparse-perl repo (both of which are explained below).

$INCLUDE parsing

$INCLUDE directives are read and parsed before the main record parsing loop to enable the included records to be parsed in the same manner as all other records.

The domain and comment parameters are supported by this new functionality however the latter is not used in the code. The domain parameter sets the relative origin of the records that are included. If excluded, the origin of the including zone is used.

The behavior of the $INCLUDE directive is to include the records of the zone file specified at the position of the directive, meaning that if the first record in the included zone has a blank name it will inherit the name of the last named record, as expected in any zone.

Added new test 'dns-zoneparse-include.t' to test the inclusion of records from another zone but without impacting the primary test. The new test is mostly a clone of dns-zoneparse.t however testing of the parsing of previously parsed and outputted records is not performed as this returns the included records being returned in an order different than when parsed directly from file or variable.

The addition of the test also saw the addition of 2 new zones, 'include-test-zone.db' which contains the $INCLUDE directives and 'included-test-zone.db' which is included by the former zone and has slight alternations in the record name and or values to distinguish the records in the test.

RFC2308 $TTL compliance

The RFC above introduces the $TTL directive that when configured causes all RRs to have their TTL set to the $TTL value if not explicitly set. This overrides the behavior of records inheriting the TTL from the last record that had it explicitly set.

The $dns_dollar_ttl variable has been introduced to store the TTL value when set by the directive and used to set the TTL of records when one is not already defined by the record.

When the $TTL directive is not used, records will use the original behavior of inheriting the last explicitly set TTL value or using the SOA record's minimum TTL.

An additional test 'dns-zoneparse-dollar-ttl.t' has been added to prove that the original behavior works when the $TTL directive is not set. The master test 'dns-zoneparse.t' has been updated to reflect the parsed result when the $TTL directive is set.

Mr Ben Nott added 4 commits October 20, 2014 09:57
Added parsing functionality before individual records are parsed such…
ed71572 
… that included records can be parsed in same loop. Added separate test 'dns-zoneparse-include.t' for testing inclusion of other zones. New test is a clone of dns-zoneparse.t but only tests parsing from data in variable and reading from file as when included records are parsed, output and parsed again they're returned in a different format than the expected hash structure due to record grouping by ORIGIN. Added 2x test zones, include-test-zone.db (performs the inclusion with the directives) and included-test-zone.db (is included by the former) as to not impact original test-zone.db. included-test-zone.db has slightly different records to distinguish results in new test. parsing is compliant with specification in RFC1035 and supports domain parameter (which sets the relative origin of the included records) and comment parameter (which isn't used at present).
Added RFC2308 compliant \$TTL directive behavior. The directive is us…
b5fb593 
…ed to set the TTL of all resource records that do not explicitly set a TTL. When a \$TTL directive is not set, the original behavior of inheriting the previously set TTL is used. When neither a \$TTL directive or explicit RR TTL is set, the minimum TTL of the SOA record is used. An additional test dns-zoneparse-dollar-ttl.t has be added to prove that the original behavior is still working. The main test dns-zoneparse.t has been updated to reflect the new behavior as the test zone test-zone.db uses a \$TTL directive. Both of these tests prove the functionality to work in either case.
Added RFC2308 compliant $TTL directive behavior.
703c732 
The directive is used to set the TTL of all resource records that do not explicitly set a TTL.
When a $TTL directive is not set, the original behavior of inheriting the previously set TTL is used.
When neither a $TTL directive or explicit RR TTL is set, the minimum TTL of the SOA record is used.
An additional test dns-zoneparse-dollar-ttl.t has be added to prove that the original behavior is still working.
The main test dns-zoneparse.t has been updated to reflect the new behavior as the test zone test-zone.db uses a $TTL directive.
Both of these tests prove the functionality to work in either case.
Merge branch 'master' of https://github.com/xdrr/dns-zoneparse-perl
df33d0a
@mschilli
Copy link
Owner

Thanks for the patch, I ran the test suite and noticed it fails on my Ubuntu system:

t/dns-zoneparse-dollar-ttl.t ....... ok     
t/dns-zoneparse-exact-soa.t ........ 1/11 
#   Failed test 'NS records parsed OK'
#   at t/dns-zoneparse-exact-soa.t line 55.
#     Structures begin differing at:
#          $got->[1]{ttl} = '1H'
#     $expected->[1]{ttl} = '43200'

#   Failed test 'NS records parsed OK'
#   at t/dns-zoneparse-exact-soa.t line 55.
#     Structures begin differing at:
#          $got->[1]{ttl} = '1H'
#     $expected->[1]{ttl} = '43200'
# Looks like you failed 2 tests of 11.
t/dns-zoneparse-exact-soa.t ........ Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/11 subtests 
t/dns-zoneparse-implied-soa.t ...... 1/11 
#   Failed test 'NS records parsed OK'
#   at t/dns-zoneparse-implied-soa.t line 55.
#     Structures begin differing at:
#          $got->[1]{ttl} = '1H'
     $expected->[1]{ttl} = '43200'

@mschilli
Copy link
Owner

Also, what is the reason for these replacements?

-                'ttl'    => '43200',
+                'ttl'    => '1H',

@mschilli
Copy link
Owner 

This test file has 2634 lines, most of which is just test data, is there a more compact way to test the functionality?

$ ls -l dns-zoneparse-include.t
89177 Oct 21 21:03 dns-zoneparse-include.t
$ wc -l dns-zoneparse-include.t
2634 dns-zoneparse-include.t

@xdrr
Copy link
Author

xdrr commented Oct 22, 2014

I was able to replicate the failing test and it looks like its due to the way in which the test includes the lib directory. Running:

/usr/bin/perl t/dns-zoneparse.t

Will cause the test to use the library ../lib as well as @inc and then attempt to include the module. ../lib from the relative path of the git repo turns into dns-zoneparse-perl/../lib which doesn't exist so the next best path is the installed version of the module.

I was able to have the test fail by removing my installed version of DNS::ZoneParse from /usr/local/share/perl (or wherever it might be for you).

I cloned your repo and noted that it has the same problem meaning the module would already have to be installed before the test will work successfully.

We can fix this by changing all of the tests to:

use lib 'lib/';

and run them from the root of the repo or we can run all the tests from within the t directory.

Please let me know what you think.

P.S. I'm working on reducing the size of the tests for the $INCLUDE functionality so it's more compact.

@xdrr
Copy link
Author

xdrr commented Oct 28, 2014

I've since updated the tests to use much smaller zone files, test only basic records and focus more closely on the $INCLUDE functionality e.g. proving that the new records are added in place of the directive and comply with the expected origin, name and TTL behavior.

I've left the use lib paths the same on all tests as I'm not sure whether you'd prefer to run tests from within the t directory or from the root of the repo.

I've updated some of the tests that existed at the time of forking as they needed to reflect the RFC2308 $TTL behavior differences (mainly that when $TTL is present the a TTL isn't specified, the TTL of a record is taken from the $TTL value and not inherited from the last explicitly mentioned TTL).

The total pull request is now down to 585 additions so please let me know what you think.

@mschilli
Copy link
Owner

Looks like the test suite is still failing:

$ perl Makefile.PL
$ make
$ make test
t/dns-zoneparse-dollar-ttl.t ....... ok
t/dns-zoneparse-exact-soa.t ........ ok
t/dns-zoneparse-implied-soa.t ...... ok
t/dns-zoneparse-include.t .......... 1/19 DNS::ZoneParse $INCLUDE references missing file: "included-test-zone.db" at t/dns-zoneparse-include.t line 28.
# Looks like you planned 19 tests but ran 1.
# Looks like your test exited with 2 just after 1.
t/dns-zoneparse-include.t .......... Dubious, test returned 2 (wstat 512, 0x200)
Failed 18/19 subtests
t/dns-zoneparse-minimum-for-ttl.t .. ok

I've left the use lib paths the same on all tests as I'm not sure whether you'd prefer to run tests from > within the t directory or from the root of the repo.

No worries, we won't require DNS::ZoneParse to be installed or the individual test case files dealing with the @inc path, we just need to make sure the test suite passes when run like shown above.

@xdrr
Copy link
Author

xdrr commented Nov 2, 2014

I've updated the 'including' zone file to use the relative path of the repo directory so that when we run

make test

the include-test-zone.db file will include the zone file with the correct path for executng the build test.

Typically, bind and other DNS service deployments aren't designed to be portable thus a full path is used in $INCLUDE directives but the way in which we use the relative path is sufficient for testing the functionality correctly.

@mschilli
Copy link
Owner

Okay, test suite succeeds now, what I'm still wondering is why you replaced so many results to make

$got->[1]{ttl} = '1H'

$expected->[1]{ttl} = '43200'

go away. This can't possibly be backward compatible, right? I mean, if someone out there relies on the result being "43200", won't their application break now that the module returns "1H"?

@xdrr
Copy link
Author

xdrr commented Nov 23, 2014

That's true. This implementation wouldn't be backward compatible if someone was relying on a particular parsing method.

Making this parsing method an option which could be activated by a subroutine or argument during construction with new() would certainly be more compatible and allow those using the module's original functionality to continue doing so.

Parsing SOA's as per RFC2308 allows this module to read zone files as most modern name servers do but as you've mentioned, the result affects the parsing of the entire zone and replaces existing functionality.

I'll add a commit to this pull request which demonstrates the functionality as an option that can be activated by subroutine call prior to parsing a zone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xdrr @mschilli